Skip to content

Update selective build example + CI for top-level targets #13741

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 11, 2025
Merged

Update selective build example + CI for top-level targets #13741

merged 1 commit into from
Sep 11, 2025

Conversation

GregoryComer
Copy link
Member

@GregoryComer GregoryComer commented Aug 27, 2025 •
edited
Loading

Summary

Update the selective build example and CI to use the new top-level targets, where appropriate. I've positioned the example as showcasing two flows - a standard way, using top-level targets, and appropriate for most use cases, and an advanced flow where the user creates a custom kernel target. This is mainly useful when integrating custom ops, which we can't easily integrate with top-level targets due to the inverted dependency on the user build.

I've updated the CMake to take an additional arg to specify whether or not to define a custom target and refactored to use the standard top-level target when not. I've also unified the CMake option naming with the top-level target. Finally, I've updated the CI job to cover both flows.

For additional context, I'd like to take a larger look at simplifying the selective build flows and CMake APIs. This will likely happen after 1.0, as it will be a larger effort.

Tracking task: #12948.

Test plan

Changes are covered by the test selective build job.

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Aug 27, 2025 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/13741

Note: Links to docs will display an error until the docs builds have been completed.

❌ 9 New Failures

As of commit f558d67 with merge base 9ffcf4b (image):

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 27, 2025
@mergennachin mergennachin requested a review from psiddh August 27, 2025 21:58
@GregoryComer GregoryComer added the release notes: none Do not include this in the release notes label Aug 27, 2025
@GregoryComer GregoryComer force-pushed the update-selective-build branch 4 times, most recently from 29111dd to aa5a45c Compare August 28, 2025 22:10
@GregoryComer GregoryComer force-pushed the update-selective-build branch 3 times, most recently from 6c2d7fa to 1c8efdf Compare September 9, 2025 03:48
@GregoryComer GregoryComer marked this pull request as ready for review September 9, 2025 04:06
@GregoryComer
Copy link
Member Author

@psiddh What are your thoughts on the updated example and README?

@psiddh
Copy link
Contributor

psiddh commented Sep 10, 2025 •
edited
Loading

@psiddh What are your thoughts on the updated example and README?

The README is much better now and clear.

  • Can you also add a note on which libs to explicitly link to ?

@GregoryComer
Copy link
Member Author

@psiddh What are your thoughts on the updated example and README?

The README is much better now and clear.

  • Can you also add a note on which libs to explicitly link to ?

Thanks. I did include this on lines 67 and 135, though it might not be discoverable enough. Do you have any suggestions for how to expose this info?

@GregoryComer GregoryComer mentioned this pull request Sep 11, 2025
Merged
GregoryComer added a commit that referenced this pull request Sep 11, 2025
@GregoryComer
Use optimized kernels when available in top-level selective build (#1…
7fbb88c 
…4203)

### Summary
Use optimized kernels for the top-level executorch_selected_kernels
target when built.

### Test plan
I've manually built with top-level selective build to verify that this
works. CI coverage will be provided with
#13741, which should land
shortly.
@GregoryComer GregoryComer merged commit 6779d15 into pytorch:main Sep 11, 2025
116 of 120 checks passed
swolchok
swolchok reviewed Sep 11, 2025
View reviewed changes
examples/selective_build/basic/CMakeLists.txt

if(NOT CMAKE_CXX_STANDARD)
set(CMAKE_CXX_STANDARD 17)
# Can't set to 11 due to executor_runner.cpp make_unique
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

executorch in general requires 17 anyway, doesn't it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we can remove this, as it's a holdover from an older version of the example. I'll clean it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swolchok swolchok swolchok left review comments

@psiddh psiddh psiddh approved these changes

@mergennachin mergennachin Awaiting requested review from mergennachin

@larryliu0820 larryliu0820 Awaiting requested review from larryliu0820 larryliu0820 is a code owner

@kirklandsign kirklandsign Awaiting requested review from kirklandsign kirklandsign is a code owner

@lucylq lucylq Awaiting requested review from lucylq lucylq is a code owner

@JacobSzwejbka JacobSzwejbka Awaiting requested review from JacobSzwejbka JacobSzwejbka is a code owner

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. release notes: none Do not include this in the release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GregoryComer @psiddh @swolchok